New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(users): custom role at profile read #6875

Open

Riddhiagrawal001 wants to merge 36 commits into main from custom-role-at-profile-read

Contributor

Riddhiagrawal001 commented Dec 18, 2024 •

edited

Loading

Type of Change

Description

This pr adds the functionality to read custom role profile read

Additional Changes

This PR modifies the API contract
This PR modifies the database schema
This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

Create custom role at profile-level
Request

curl --location 'http://localhost:9000/api/user/role' \
--header 'authorization: Bearer JWT' \
--data '{
    "role_scope": "merchant",
    "groups": [
        "operations_manage",
        "connectors_view"
    ],
    "role_name": "profile-level-custom-role",
    "entity_type":"profile"
}'

Response

{
    "error": {
        "type": "invalid_request",
        "message": "User Role Operation Not Supported",
        "code": "UR_23"
    }
}

Older flows should work as before . Changes for create profile custom roles will be in consecutive pr
Validate custom role
If a role with the same name is created either below me or in my lineage , then i wouldn't be able to create the role with that same name
Request

curl 'http://localhost:9000/api/user/role' \
  -H 'Accept: */*' \
  -H 'authorization: Bearer JWT' \
  --data-raw '{"role_scope":"merchant","groups":["operations_view","operations_manage"],"role_name":"m2-org"}'

Response

{
    "error": {
        "type": "invalid_request",
        "message": "Role name already exists",
        "code": "UR_35"
    }
}

Checklist

I formatted the code cargo +nightly fmt --all
I addressed lints thrown by cargo clippy
I reviewed the submitted code
I added unit tests for my changes where possible

Riddhiagrawal001 and others added 24 commits

November 28, 2024 15:44


          chore: rename find_role_by_role_id_in_org_scopr

9c04ad6


          refactor(users): refactor find_by_role_id_in_merchant_scope query for…

35b89e8

… custom roles


          fix: changes for list roles and list roles at entity-level

5cb5954


          Merge branch 'main' into change-roles-query

16f6f59


          Merge branch 'main' into change-roles-query

d18866e


          fix: changes for backfilling user-roles


          Merge branch 'change-roles-query' of https://github.com/juspay/hypers…

…witch into change-roles-query


          Merge branch 'main' into change-roles-query

aeec4bf


          fix: addressed comments

1cd045f


          Merge branch 'change-roles-query' of https://github.com/juspay/hypers…

af27d60

…witch into change-roles-query


          refactor: code refactoring

2b24810


          Merge branch 'main' into change-roles-query

989b423


          merge branch:synced with main

5a96d2e


          Merge branch 'change-roles-query' of https://github.com/juspay/hypers…

8e38e66

…witch into change-roles-query


          fix: addressed comments

f70f3cf


          Merge branch 'main' into change-roles-query

b3de586


          Merge branch 'change-roles-query' into custom-role-at-profile-read

55b3ecf


          Merge branch 'main' into change-roles-query

bc4a038


          Merge branch 'main' of https://github.com/juspay/hyperswitch into cha…

f307fee

…nge-roles-query


          Merge branch 'change-roles-query' of https://github.com/juspay/hypers…

26ff9ac

…witch into change-roles-query


          Merge branch 'change-roles-query' into custom-role-at-profile-read

5a428b4


          changes-for-profile-read

abeac9c


          Merge branch 'main' of https://github.com/juspay/hyperswitch into cus…

22ee8f0

…tom-role-at-profile-read


          refactor: removed unused code

335b359

Riddhiagrawal001 requested review from a team as code owners

December 18, 2024 07:41

semanticdiff-com bot commented Dec 18, 2024 •

edited

Loading

Review changes with

Changed Files

File	Status
crates/router/src/core/user_role/role.rs	26% smaller
crates/router/src/db/kafka_store.rs	19% smaller
crates/router/src/utils/user_role.rs	15% smaller
crates/diesel_models/src/query/role.rs	9% smaller
crates/common_enums/src/enums.rs	6% smaller
crates/router/src/db/role.rs	5% smaller
crates/api_models/src/user_role/role.rs	0% smaller
crates/diesel_models/src/role.rs	0% smaller
crates/diesel_models/src/schema.rs	0% smaller
crates/diesel_models/src/schema_v2.rs	0% smaller
crates/router/src/core/user.rs	0% smaller
crates/router/src/core/user_role.rs	0% smaller
crates/router/src/services/authorization/roles.rs	0% smaller
migrations/2024-10-17-073555_add-profile-id-to-roles/down.sql	Unsupported file format
migrations/2024-10-17-073555_add-profile-id-to-roles/up.sql	Unsupported file format
migrations/2024-10-17-123943_add-profile-enum-in-role-scope/down.sql	Unsupported file format
migrations/2024-10-17-123943_add-profile-enum-in-role-scope/up.sql	Unsupported file format
migrations/2024-12-18-061400_change-roles-index/down.sql	Unsupported file format
migrations/2024-12-18-061400_change-roles-index/up.sql	Unsupported file format

hyperswitch-bot bot added the M-database-changes label


          chore: run formatter

26eaa33

Riddhiagrawal001 added 2 commits

December 18, 2024 14:46


          fix: added indexes

cfbeeaf


          Merge branch 'custom-role-at-profile-read' of https://github.com/jusp…

40cf084

…ay/hyperswitch into custom-role-at-profile-read

Riddhiagrawal001 changed the title ~~fix(users): custom role at profile read~~ feat(users): custom role at profile read

Riddhiagrawal001 self-assigned this


          fix: variable name changes

04703e3

Riddhiagrawal001 linked an issue

that may be closed by this pull request

feat(users): Add profile level custom role #6488

Open

ThisIsMani reviewed

View reviewed changes

crates/router/src/core/user_role/role.rs Outdated

Comment on lines 100 to 105

+                  } else if requestor_entity_from_role_scope < role_entity_type {
+                      return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!(
+                          "User is trying to create role of type {} and scope {}",
+                          requestor_entity_from_role_scope, role_entity_type
+                      ));
+                  }

Contributor

ThisIsMani Dec 19, 2024

This should be checked first.

crates/router/src/core/user_role/role.rs Outdated

Comment on lines 120 to 121

		let profile_id =
		matches!(role_entity_type, EntityType::Profile).then_some(user_from_token.profile_id);

Contributor

ThisIsMani Dec 19, 2024

Make this a match instead.

crates/router/src/core/user_role/role.rs Outdated

Comment on lines 90 to 105

+                  if user_entity_type < role_entity_type {
+                      return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!(
+                          "{} is trying to create {} ",
+                          user_entity_type, role_entity_type
+                      ));
+                  } else if user_entity_type < requestor_entity_from_role_scope {
+                      return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!(
+                          "{} is trying to create role of scope {} ",
+                          user_entity_type, requestor_entity_from_role_scope
+                      ));
+                  } else if requestor_entity_from_role_scope < role_entity_type {
+                      return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!(
+                          "User is trying to create role of type {} and scope {}",
+                          requestor_entity_from_role_scope, role_entity_type
+                      ));
+                  }

Contributor

ThisIsMani Dec 19, 2024

Separate these ifs.

crates/router/src/core/user_role/role.rs Outdated

Comment on lines 247 to 249

+                          "{} is trying to update {} ",
+                          user_role_info.get_entity_type(),
+                          role_info.get_entity_type()

Contributor

ThisIsMani Dec 19, 2024

Print scope as well here.

crates/router/src/core/user_role/role.rs Outdated

+                  let user_role_info = user_from_token.get_role_info_from_db(&state).await?;
+                  let max_from_scope_and_entity = cmp::max(
+                      user_role_info.get_entity_type(),

Contributor

ThisIsMani Dec 19, 2024

Should be taken from role_info.

crates/router/src/db/role.rs Outdated

Comment on lines 268 to 273

+                                          .profile_id
+                                          .as_ref()
+                                          .map(|profile_id_from_role| {
+                                              profile_id_from_role == profile_id
+                                                  && role.scope == RoleScope::Profile
+                                          })

Contributor

ThisIsMani Dec 19, 2024

Use is_some_and.

crates/diesel_models/src/query/role.rs Outdated

Comment on lines 287 to 289

+                              DieselError::NotFound => {
+                                  Err(report!(err)).change_context(errors::DatabaseError::NotFound)
+                              }

Contributor

ThisIsMani Dec 20, 2024

Will this throw error if nothing is found?

Contributor Author

Riddhiagrawal001 Dec 23, 2024

This case wont come as it will resolve in Ok when value is [] . Hence will remove this

crates/router/src/db/role.rs Outdated

-                      Ok(roles_list)
+                      roles_list.ok_or(
+                          errors::StorageError::ValueNotFound(
+                              "No role available with role_name={role_name} in org_id = {org_id:?}".to_string(),

Contributor

ThisIsMani Dec 20, 2024

Maybe printing the EntityTypePayload is more appropriate here.

crates/diesel_models/src/query/role.rs Outdated

Comment on lines 226 to 228

+                                          .eq(RoleScope::Organization)
+                                          .or(dsl::scope.eq(RoleScope::Merchant))
+                                          .or(dsl::scope.eq(RoleScope::Profile)),

Contributor

ThisIsMani Dec 20, 2024

Should we make scopes also a Vec? Will that improve readability?

Converting to Vec will give us an advantage, we don't have to change the query if scopes are added or removed. But I assume, we won't be changing scopes anytime soon.

Contributor Author

Riddhiagrawal001 Dec 23, 2024

updated the query with the discussed changes

crates/diesel_models/src/query/role.rs Outdated

Comment on lines 109 to 111

+                                      EntityType::Profile,
+                                      EntityType::Merchant,
+                                      EntityType::Organization,

Contributor

ThisIsMani Dec 20, 2024

These entity types should be based on the Payload's entity type.

If we run this query for profile level Payload, we will also search for org level entity_types which is wrong (correct me if I am wrong).

And if it is not based on the payload, then this query is technically a subset for the list query.

Contributor Author

Riddhiagrawal001 Dec 23, 2024

Its been modified to check with list query

Riddhiagrawal001 and others added 8 commits

December 23, 2024 13:21


          fix: query changes

6a35c22


          refactor: addressed comments

f8f5936


          fix: addressed comments

0f7acd6


          refactor: refactor code

142ff92


          Merge branch 'main' of https://github.com/juspay/hyperswitch into cus…

10c5809

…tom-role-at-profile-read


          chore: run formatter

b1be688


          refactor: spell correction

7fd671b


          Merge branch 'custom-role-at-profile-read' of https://github.com/jusp…

81336c1

…ay/hyperswitch into custom-role-at-profile-read

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M-database-changes